Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #2139. Add generator functions element type tests #2153

Merged
merged 6 commits into from
Aug 10, 2023

Conversation

sgrekhov
Copy link
Contributor

No description provided.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I do have a few comments.

There are many tests with a description similar to "Check that it is a compile-time error if an element type of a synchronous generator function f is not U, where S is a union-free type of the declared return type of f and S implements Iterable<U>"

I can't read that description. ;-)

First, there's no such thing as 'an element type' of a generator function, there is one specific type which is 'the element type' of that function. Next 'is not U' is far too flexible: There is an infinity of ways in which we can make the requirements true, and still satisfy 'is not U'.

So we'd have to say something like this:

"Check that if the declared return type of a synchronous generator function f is T, and S is the union-free type derived from T, and S is of the form Iterable<U>, it is a compile-time error to yield an expression whose static type isn't assignable to U."

But that's just the same thing as this:

"Check that for a synchronous generator function f with element type U which is not dynamic, it is a compile-time error to yield an expression whose static type isn't assignable to U."

With that, the 'asynchronous' variant would be achieved simply by replacing a space by 'n a'. ;-)

I think all the descriptions containing 'an element type' needs a similar adjustment, but please check all descriptions (I didn't confirm that they always contain 'an element type').

Next, there are many tests where we need to check that a given object has a run-time type that implements Iterable<T> for a specific T. Here is a way to perform that check:

// Testing method.

bool runtimeTypeImplementsIterableOf<X>(Object? o) {
  if (o is! Iterable<X>) return false;
  List<X> list = o.toList(growable: true);
  try {
    list.addAll(<X>[]);
  } on TypeError catch (_) {
    return false;
  }
  return true;
}

// Example usage.

void main() {
  Iterable<dynamic> xs = <num>[1];

  print(xs.runtimeType == List<num>); // 'false', it is some subtype.
  print(runtimeTypeImplementsIterableOf<Object>(xs)); // 'false'.
  print(runtimeTypeImplementsIterableOf<num>(xs)); // 'true'.
  print(runtimeTypeImplementsIterableOf<int>(xs)); // 'false'.
}

First, we can't use the runtimeType R of the target object o for anything: It's going to be some subtype of the statically known type, and it certainly won't be Iterable or List because they are abstract. Next, we can establish that R <: Iterable<X>. Finally, we need to ensure that the type argument of R at Iterable isn't a more special type (e.g., we wouldn't want to accept an object with run-time type that implements Iterable<Never>). We can do that by getting hold of the corresponding List (so it won't work if it is an infinite iterable, but I think we can handle that), and then test addAll.

I mentioned toList and addAll in several tests, but it's of course much better to have a standard testing function like the one above.

The current version of iterableElementsRuntimeIs might be trying to do a similar thing, but it won't work.

Language/Functions/element_type_A01_t01.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t01.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t02.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t03.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t04.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A03_t02.dart Outdated Show resolved Hide resolved
Utils/expect_common.dart Outdated Show resolved Hide resolved
Utils/tests/Expect/iterableElementRuntimeIs_A01_t01.dart Outdated Show resolved Hide resolved
Utils/tests/Expect/iterableElementRuntimeIs_A01_t02.dart Outdated Show resolved Hide resolved
Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests updated. I renamed checking functions in Expect to Expect.isRuntimeTypeIterable and Expect.isNotRuntimeTypeIterable. Looks like this naming is more in consistance with the dart style. If you have better namig ideas, please let me know

I also preserved throwing exceptions by these functions. If we change it to return bool the we'll have to use Expect.isTrue(Expect.(isRuntimeTypeIterable(...)));. First, we never write a code this way (Expect.foo(Expect.baz(...))). Second, in case of failure we'll get an error Expect.isTrue(false) fails.. Expect.isRuntimeTypeIterable<int>(o); produces Not Iterable<int>: _SyncStarIterable<dynamic> which is much more informative

Language/Functions/element_type_A01_t01.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t01.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t02.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t03.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t04.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t06.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A02_t03.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A02_t04.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A02_t05.dart Show resolved Hide resolved
Language/Functions/element_type_A02_t06.dart Show resolved Hide resolved
@sgrekhov sgrekhov requested a review from eernstg August 9, 2023 08:40
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still some things to think about. ;-)

I think it's really important to make an explicit distinction between o is Iterable<T> for some given T, and "the run-time type of o implements Iterable<T>", because the latter is strictly more demanding than the former. Based on that consideration, I've suggested a couple of renames.

Given that we're using a strong test, there is probably no need for isNotRuntimeTypeIterable at all, so I've suggested that it gets deleted.

Finally, a couple of descriptions still use the phrase 'an element type' of a function, or some other wording that is confusing.

Utils/expect_common.dart Outdated Show resolved Hide resolved
Utils/expect_common.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t04.dart Outdated Show resolved Hide resolved
Expect.isNotRuntimeTypeIterable<Object>(f1());
Expect.isNotRuntimeTypeIterable<Object?>(f1());
Expect.isNotRuntimeTypeIterable<dynamic>(f1());
Expect.isRuntimeTypeIterable<int?>(f1());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might as well delete lines 36-40 (including both), because a successful execution of line 41 implies a successful execution of the others. Similarly for lines 43-48, and for A01_t06.

import "dart:async";
import "../../Utils/expect.dart";

void isRuntimeTypeStream<T>(Object? o) async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

Perhaps rename to include the word Implements as well.

Language/Functions/element_type_A02_t05.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A02_t06.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A03_t01.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t05.dart Outdated Show resolved Hide resolved
Language/Functions/element_type_A01_t06.dart Outdated Show resolved Hide resolved
@sgrekhov
Copy link
Contributor Author

sgrekhov commented Aug 9, 2023

Tests updated. Testing function is renamed to include Implements and ...Not... version deleted. Please take another look.

@sgrekhov sgrekhov requested a review from eernstg August 9, 2023 13:39
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@eernstg eernstg merged commit ccd6eae into dart-lang:master Aug 10, 2023
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 16, 2023
2023-08-16 [email protected] Fixes dart-lang/co19#2211. Fix roll failures (dart-lang/co19#2212)
2023-08-16 [email protected] dart-lang/co19#2142. Add more dynamic semantics and member invocation tests (dart-lang/co19#2210)
2023-08-15 [email protected] dart-lang/co19#2142. Delete obsolete Inline-classes tests (dart-lang/co19#2209)
2023-08-14 [email protected] dart-lang/co19#2207. Fix roll failures (dart-lang/co19#2208)
2023-08-14 [email protected] Fixes dart-lang/co19#2195. Fix typos and a runtime error in static_analysis_member_invocation_A07_t01.dart (dart-lang/co19#2204)
2023-08-14 [email protected] Fixes dart-lang/co19#2196. Expect additional error in syntax_A04_t01.dart (dart-lang/co19#2205)
2023-08-14 [email protected] dart-lang/co19#2142. Move and rename dynamic semantics extension type tests (dart-lang/co19#2206)
2023-08-14 [email protected] Fixes dart-lang/co19#2190. Fix representation dependency in static_analysis_extension_types_A19_t01.dart (dart-lang/co19#2198)
2023-08-14 [email protected] Fixes dart-lang/co19#2194. Fix typo in static_analysis_extension_types_A21_t06.dart (dart-lang/co19#2203)
2023-08-14 [email protected] Fixes dart-lang/co19#2191. Fix typos in static_analysis_extension_types_A21_t01.dart (dart-lang/co19#2199)
2023-08-14 [email protected] Fixes dart-lang/co19#2189. Fix typos in extension types tests (dart-lang/co19#2200)
2023-08-14 [email protected] Fixes dart-lang/co19#21892. Fix typo in static_analysis_extension_types_A21_t02.dart (dart-lang/co19#2201)
2023-08-14 [email protected] Fixes dart-lang/co19#21892. Fix typo in static_analysis_extension_types_A21_t05.dart (dart-lang/co19#2202)
2023-08-11 [email protected] Fixes dart-lang/co19#2172. Fix error expectations in static_analysis_extension_types_A02_t02.dart (dart-lang/co19#2180)
2023-08-11 [email protected] dart-lang/co19#2142. Change dynamic semantics inline class tests to be extension type tests (dart-lang/co19#2188)
2023-08-11 [email protected] dart-lang/co19#2142. Move static analysis member invocation tests to Extension types dir (dart-lang/co19#2187)
2023-08-11 [email protected] Fixes dart-lang/co19#2174. Add missing import (dart-lang/co19#2182)
2023-08-11 [email protected] dart-lang/co19#2142. Rename and move tests to Extension type dir (dart-lang/co19#2186)
2023-08-11 [email protected] Fixes dart-lang/co19#2175. Fix tests that check extension types implementing enumerated types (dart-lang/co19#2183)
2023-08-11 [email protected] Fixes dart-lang/co19#2176. Fix unintended error in static_analysis_extension_types_A11_t01.dart (dart-lang/co19#2184)
2023-08-11 [email protected] Fixes dart-lang/co19#2177. Fix typo in static_analysis_extension_types_A16_t05.dart (dart-lang/co19#2185)
2023-08-11 [email protected] Fixes dart-lang/co19#2173. Fix tests checking use of await for extension types (dart-lang/co19#2181)
2023-08-11 [email protected] Fixes dart-lang/co19#2171. Fix typos, adjust error expectations (dart-lang/co19#2179)
2023-08-10 [email protected] dart-lang/co19#2142. Change composing inline class tests to be extension type tests (dart-lang/co19#2170)
2023-08-10 [email protected] Fixes dart-lang/co19#2167. Fix typo in static_analysis_extension_types_A17_t01.dart (dart-lang/co19#2168)
2023-08-10 [email protected] Fixes dart-lang/co19#2139. Add generator functions element type tests (dart-lang/co19#2153)
2023-08-09 [email protected] dart-lang/co19#2142. Change static analysis member invocation tests to be extension type tests (dart-lang/co19#2150)
2023-08-08 [email protected] dart-lang/co19#2142. Add more static analysis of extension types tests (dart-lang/co19#2166)
2023-08-08 [email protected] dart-lang/co19#2142. Add more static analysis tests (dart-lang/co19#2165)

Change-Id: Ib1f0a9a1f2615b9f9783b1d7fa1b60c4d6c0389c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/321140
Commit-Queue: Erik Ernst <[email protected]>
Reviewed-by: Erik Ernst <[email protected]>
@sgrekhov sgrekhov deleted the co19-2139 branch November 14, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants